-
-
Notifications
You must be signed in to change notification settings - Fork 21k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add a method to construct a NodePath from a StringName #72702
base: master
Are you sure you want to change the base?
Conversation
I just ran into a situation where code that worked fine in 3.x doesn't due to this. Can this get merged into one of the patch releases? |
536d2b5
to
8f31602
Compare
8f31602
to
1c44cf2
Compare
1c44cf2
to
79f230d
Compare
Perhaps we just need to add the godot/core/variant/variant.cpp Lines 558 to 563 in e74bf83
godot/core/variant/variant.cpp Lines 714 to 718 in e74bf83
godot/core/variant/variant.cpp Lines 722 to 726 in e74bf83
|
79f230d
to
25c5898
Compare
@dalexeev Good idea, pushed a change to add that, but we should also keep the method exposed for explicitness. |
See also: godot/core/variant/variant_construct.cpp Lines 79 to 82 in fd31dc7
godot/core/variant/variant_construct.cpp Lines 176 to 182 in fd31dc7
Perhaps it makes sense to add constructors only in GDScript, even if we can't add them in C++? |
269b481
to
3475d55
Compare
3475d55
to
21f28d8
Compare
21f28d8
to
0f23e61
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good to me.
From the perspective of the C# bindings, I wonder if it'd be better to provide a static method or constructor that takes the StringName and just creates the NodePath assuming it contains no slashes or colons.
This would be an "unsafe" way to create a NodePath from StringName but I wouldn't expose this in the C# bindings. It would be used internally to create the NodePath when we can guarantee that the StringName doesn't contain slashes or colons. This means the logic to check this would be done in C# directly which would allow us to use IndexOfAny
which is vectorized.
However, using IndexOfAny
would require us to convert the StringName to a C# string first and that means creating a NodePath would take 2 interop calls instead of 1. I haven't benchmarked any of this so I don't know what would be better. It's possible that vectorizing the check for slashes and colons wouldn't bring much improvement over using this PR's static method, and I guess it's also possible to vectorize the C++ code in the future.
So, having said all that, I think this PR is still an improvement over the current way to create NodePaths from StringNames. I wonder if other language bindings authors are also interested in this API and would use it to either expose a way to create NodePaths from StringNames or, if it already exists, to improve their current implementation.
cc @Bromeon
0f23e61
to
974c0ae
Compare
974c0ae
to
a8fbe09
Compare
a8fbe09
to
6690df6
Compare
6690df6
to
e1c9aea
Compare
// Check if p_string_name contains a slash or colon. If so, we need to parse it. | ||
const char32_t *chars = string.ptr(); | ||
for (int i = 0; i < size; i++) { | ||
if (unlikely(chars[i] == '/' || chars[i] == ':')) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes me think it'd be interesting to see if we should add a findmc
to complement findmk
, and perhaps a contains_any
or similar, there's many places that look for "is any of these present" and it'd be a good feature, just to think about I might look at an implementation later
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wouldn't that require constructing a Vector<char32_t>
, so less efficient than as it is now?
But in general, that seems like a good idea.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For two arguments I'd tend to agree
e1c9aea
to
d2c2b7b
Compare
d2c2b7b
to
55433db
Compare
55433db
to
1210349
Compare
1210349
to
e317196
Compare
e317196
to
c82bfb7
Compare
c82bfb7
to
f4276f1
Compare
f4276f1
to
bf0112f
Compare
bf0112f
to
648bbf7
Compare
This allows constructing a NodePath directly from a StringName, which skips the String parsing logic, if the given StringName does not contain a slash or colon and can be used as-is.
I considered a constructor, but this would break all uses in the C++ code that construct NodePath from
char *
literals, and we need to depend on two other constructors anyway, so this uses a static method instead.